Sidebar adjustments - #36
Conversation
… include Investigator role
…urity roles for ML service endpoints
…d and embedding generation
…add default bucket configuration in application.properties
There was a problem hiding this comment.
Pull request overview
This PR adds criminal record CRUD functionality with Supabase photo storage, face embedding generation, and expanded criminal profile fields (crime history, address, contacts, DOB, gender, alias).
Changes:
- New
CriminalControllerandCriminalServicefor full CRUD on criminal records with photo upload to Supabase and ML embedding generation - New
SupabaseStorageServicefor uploading/deleting files in Supabase Storage - Extended
Criminalentity,FacialRecognitionService, andSecurityConfigwith additional fields and role-based access
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
CriminalController.java |
New REST controller for criminal CRUD endpoints |
CriminalService.java |
New service orchestrating DB ops, storage uploads, and embeddings |
SupabaseStorageService.java |
New service for Supabase Storage file upload/delete |
Criminal.java |
Added new profile fields (riskLevel, crimeHistory, photoUrl, DOB, gender, alias) |
CriminalRepository.java |
Added native query for IDs with face embeddings |
FacialRecognitionService.java |
Added generateEmbedding method; expanded registerCriminal params; renamed photo → photos |
FacialRecognitionController.java |
Extended registration endpoint with additional profile params |
SecurityConfig.java |
Added role-based access for new criminal and facial endpoints |
application.properties |
Added supabase.bucket config property |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| .requestMatchers("/api/criminals/**").hasAnyRole("Investigator", "OIC") | ||
| .requestMatchers("/api/criminals").hasAnyRole("Investigator", "OIC") |
There was a problem hiding this comment.
Security rule ordering issue: /api/criminals/** is matched before /api/criminals due to Spring Security's first-match-wins behavior with requestMatchers. However, the more significant problem is that /api/criminals/** won't match the bare /api/criminals path (no trailing slash). The rules should be reordered so /api/criminals comes before /api/criminals/**, though in practice both have the same role set here so the functional impact is limited. Still, swap the order for correctness and consistency.
| .requestMatchers("/api/criminals/**").hasAnyRole("Investigator", "OIC") | |
| .requestMatchers("/api/criminals").hasAnyRole("Investigator", "OIC") | |
| .requestMatchers("/api/criminals").hasAnyRole("Investigator", "OIC") | |
| .requestMatchers("/api/criminals/**").hasAnyRole("Investigator", "OIC") |
| private final RestTemplate restTemplate = new RestTemplate(); | ||
|
|
There was a problem hiding this comment.
This creates a new RestTemplate instance directly instead of using the Spring-managed RestTemplate bean (configured with timeouts in RestTemplateConfig). This means uploads will use default infinite timeouts, which could cause threads to hang indefinitely if Supabase is unresponsive. Inject the existing RestTemplate bean via constructor injection instead.
| private final RestTemplate restTemplate = new RestTemplate(); | |
| private final RestTemplate restTemplate; | |
| public SupabaseStorageService(RestTemplate restTemplate) { | |
| this.restTemplate = restTemplate; | |
| } |
| listHeaders.set("apikey", supabaseServiceKey); | ||
| listHeaders.setContentType(MediaType.APPLICATION_JSON); | ||
|
|
||
| String listBody = "{\"prefix\":\"" + criminalId + "/\",\"limit\":100}"; |
There was a problem hiding this comment.
The criminalId is concatenated directly into the JSON string without escaping. If criminalId contains characters like " or \, this will produce malformed JSON or could be exploited for JSON injection. Use the ObjectMapper to serialize the body safely, as is done later in the method for deleteBody.
| String storagePath = criminalId + "/" + safeFilename; | ||
|
|
||
| String uploadUrl = supabaseUrl + "/storage/v1/object/" + bucket + "/" + storagePath; |
There was a problem hiding this comment.
The criminalId is interpolated directly into URL paths without encoding. If the ID contains special characters (e.g., /, %, ?), it could break the URL or lead to path traversal. Although currently generated as a UUID substring (safe), the uploadPhoto method accepts an arbitrary String. Consider URL-encoding the path segments.
| String deleteUrl = supabaseUrl + "/storage/v1/object/" + bucket; | ||
|
|
||
| HttpHeaders deleteHeaders = new HttpHeaders(); | ||
| deleteHeaders.set("Authorization", "Bearer " + supabaseServiceKey); | ||
| deleteHeaders.set("apikey", supabaseServiceKey); | ||
| deleteHeaders.setContentType(MediaType.APPLICATION_JSON); | ||
|
|
||
| String deleteBody = mapper.writeValueAsString(new java.util.LinkedHashMap<String, Object>() {{ | ||
| put("prefixes", prefixes); | ||
| }}); | ||
| HttpEntity<String> deleteRequest = new HttpEntity<>(deleteBody, deleteHeaders); | ||
|
|
||
| ResponseEntity<String> deleteResponse = restTemplate.exchange( | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); |
There was a problem hiding this comment.
The Supabase bulk delete endpoint is POST /storage/v1/object/remove/{bucket}, not DELETE /storage/v1/object/{bucket}. The current URL and HTTP method appear incorrect — this will likely result in a 404 or 405 from Supabase. The URL should be supabaseUrl + "/storage/v1/object/remove/" + bucket and the method should be HttpMethod.POST (or HttpMethod.DELETE with the correct path, depending on the Supabase API version).
| .requestMatchers("/api/facial/criminals").hasAnyRole("Investigator", "OIC") | ||
| .requestMatchers("/api/facial/**").hasRole("Investigator") | ||
|
|
||
| // Criminal CRUD (direct DB, no Python) |
There was a problem hiding this comment.
The comment says "Criminal CRUD (direct DB, no Python)" but CriminalService.createCriminal and updateCriminal both call facialRecognitionService.generateEmbedding(), which forwards to the Python ML service. The comment is misleading.
| // Criminal CRUD (direct DB, no Python) | |
| // Criminal CRUD endpoints |
No description provided.